Skip to content

Conversation

@guillep2k
Copy link
Member

This PR fixes a panic produced while creating reviews, and adds review comments to the notification mail template.

@guillep2k guillep2k changed the title Fix review notifications and add them to mail templates WIP: Fix review notifications and add them to mail templates Nov 13, 2019
@guillep2k
Copy link
Member Author

@lunny I've marked the PR WIP for your consideration, although it's already functional.

It turns out that the review service was the wrong place to add the notifications, as they were already being added a few lines below where the comment is created.

What I'd suggest to follow the "services" model is to move most of routers/repo/pull_review.go to services/pull/review.go (which I removed because it was only calling models now). Perhaps that can be done in a future PR to keep this PR simple.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 13, 2019
@codecov-io
Copy link

Codecov Report

Merging #8948 into master will increase coverage by 0.01%.
The diff coverage is 18.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8948      +/-   ##
==========================================
+ Coverage   41.23%   41.25%   +0.01%     
==========================================
  Files         547      546       -1     
  Lines       70794    70801       +7     
==========================================
+ Hits        29195    29207      +12     
+ Misses      37880    37870      -10     
- Partials     3719     3724       +5
Impacted Files Coverage Δ
models/issue_comment.go 48.07% <ø> (+0.21%) ⬆️
routers/repo/pull_review.go 0% <0%> (ø) ⬆️
modules/notification/mail/mail.go 26.56% <0%> (-2.26%) ⬇️
services/mailer/mail.go 43.88% <26.08%> (-2.42%) ⬇️
modules/migrations/migrate.go 21.22% <0%> (-1.68%) ⬇️
modules/migrations/gitea.go 8.05% <0%> (-0.64%) ⬇️
models/repo.go 46.46% <0%> (-0.05%) ⬇️
models/repo_list.go 74.07% <0%> (+0.92%) ⬆️
models/error.go 33.86% <0%> (+1.18%) ⬆️
modules/task/migrate.go 28.94% <0%> (+3.94%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b97e04...86f906a. Read the comment docs.

@guillep2k
Copy link
Member Author

Example review mail:

image

@guillep2k
Copy link
Member Author

Close in favor of #8954
I can add the template stuff later, since the work is already done. 😁

@guillep2k guillep2k closed this Nov 13, 2019
@guillep2k guillep2k deleted the fix-reviews branch November 20, 2019 13:08
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants